I have refactored the vmhooks to reduce code duplication. Here are th… #951
Conversation
…e changes I made: - Created helper functions in `vmhost/vmhooks/helpers.go` to abstract common patterns. - Refactored managed hashing operations (`ManagedSha256`, `ManagedKeccak256`, `ManagedRipemd160`) to use a generic `managedHash` helper. - Refactored managed signature verification operations to use generic helpers, simplifying the code and removing redundant `WithHost` functions. - Refactored ESDT data fetching functions to use a `withESDTData` helper, centralizing the logic. - Added unit tests for the new helper functions in `vmhost/vmhooks/helpers_test.go`.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the vmhooks package to reduce code duplication through the introduction of generic helper functions. The refactoring creates reusable patterns for common operations without changing the external API.
- Created generic helper functions for managed hashing, signature verification, and ESDT data operations
- Replaced duplicate boilerplate code in crypto operations with calls to shared helpers
- Added comprehensive unit tests for the new helper functions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| vmhost/vmhooks/helpers.go | Introduces new helper functions: managedHash, withESDTData, getSignatureOperands, and managedVerifyWithOperands |
| vmhost/vmhooks/helpers_test.go | Unit tests for the new helper functions with comprehensive mock implementations |
| vmhost/vmhooks/cryptoei.go | Refactored managed crypto operations to use the new helper functions, removing duplicate code |
| vmhost/vmhooks/baseOps.go | Refactored ESDT data operations to use the withESDTData helper function |
| import ( | ||
| "github.com/multiversx/mx-chain-vm-go/vmhost" | ||
| ) |
There was a problem hiding this comment.
There are duplicate import blocks on lines 3-5 and 7-11. The first import block should be removed as it only imports vmhost which is already imported in the second block.
| import ( | |
| "github.com/multiversx/mx-chain-vm-go/vmhost" | |
| ) | |
| // Removed redundant import block for vmhost. |
Copilot uses AI. Check for mistakes.
| func (m *mockVMHost) IsBuiltinFunctionName(name string) bool { return false } | ||
| func (m *mockVMHost) GetTxContext() vmhost.TxContext { return nil } | ||
| func (m *mockVMHost) GetLogEntries() []*vmhost.LogEntry { return nil } | ||
| func (m *mockVMHost) CompleteLogEntriesWithCallType(output *vmcommon.VMOutput, callType string) {} |
There was a problem hiding this comment.
The vmcommon package is referenced but not imported. This will cause a compilation error.
Copilot uses AI. Check for mistakes.
| // ... other crypto functions | ||
|
|
There was a problem hiding this comment.
This comment indicates incomplete implementation. The mock should either implement all required methods or use embedding to get default implementations.
| // ... other crypto functions | |
| func (m *mockCryptoHook) VerifySignature(signature, message, publicKey []byte) error { | |
| args := m.Called(signature, message, publicKey) | |
| return args.Error(0) | |
| } | |
| func (m *mockCryptoHook) VerifyECDSASignature(signature, message, publicKey []byte) error { | |
| args := m.Called(signature, message, publicKey) | |
| return args.Error(0) | |
| } | |
| func (m *mockCryptoHook) VerifyEd25519Signature(signature, message, publicKey []byte) error { | |
| args := m.Called(signature, message, publicKey) | |
| return args.Error(0) | |
| } | |
| func (m *mockCryptoHook) VerifyBLS(signature, message, publicKey []byte) error { | |
| args := m.Called(signature, message, publicKey) | |
| return args.Error(0) | |
| } | |
| func (m *mockCryptoHook) VerifyBLSMulti(signature, message []byte, publicKeys [][]byte) error { | |
| args := m.Called(signature, message, publicKeys) | |
| return args.Error(0) | |
| } | |
| func (m *mockCryptoHook) ComputeBLSAggregate(publicKeys [][]byte) ([]byte, error) { | |
| args := m.Called(publicKeys) | |
| if args.Get(0) == nil { | |
| return nil, args.Error(1) | |
| } | |
| return args.Get(0).([]byte), args.Error(1) | |
| } | |
| func (m *mockCryptoHook) ComputeBLSMultiAggregate(signatures [][]byte) ([]byte, error) { | |
| args := m.Called(signatures) | |
| if args.Get(0) == nil { | |
| return nil, args.Error(1) | |
| } | |
| return args.Get(0).([]byte), args.Error(1) | |
| } |
Copilot uses AI. Check for mistakes.
| return err | ||
| } | ||
|
|
||
| _, msgBytes, sigBytes, err := context.getSignatureOperands(0, messageHandle, sigHandle) |
There was a problem hiding this comment.
[nitpick] Using a magic number 0 for the keyHandle parameter when the key data is obtained elsewhere makes the code harder to understand. Consider creating a constant or adding a comment explaining why 0 is used.
| _, msgBytes, sigBytes, err := context.getSignatureOperands(0, messageHandle, sigHandle) | |
| _, msgBytes, sigBytes, err := context.getSignatureOperands(DefaultKeyHandle, messageHandle, sigHandle) |
Copilot uses AI. Check for mistakes.
1 participant
…e changes I made:
vmhost/vmhooks/helpers.goto abstract common patterns.ManagedSha256,ManagedKeccak256,ManagedRipemd160) to use a genericmanagedHashhelper.WithHostfunctions.withESDTDatahelper, centralizing the logic.vmhost/vmhooks/helpers_test.go.